fix(asr): repair ConvSubsampling forward paths missed by the MaskedConvSequential refactor - #16225
ManoharPaturi wants to merge 1 commit into
Conversation
…nvSequential refactor The MaskedConvSequential refactor left three ConvSubsampling paths broken: 1. `subsampling_conv_chunking_factor=-1` (chunking disabled, documented in the class docstring) falls into `x, lengths = self.conv(x)`, which calls MaskedConvSequential.forward without the required `lengths` argument and raises TypeError for every conv2d stack (vggnet, striding, dw_striding). 2. The 1-D stacks (striding_conv1d, dw_striding_conv1d, conv2d_subsampling=False) always take that same branch regardless of the chunking factor, so both variants raise TypeError on every forward pass. 3. For vggnet, `_forward_torch` reads `layer.kernel_size[0]` / `layer.stride[0]`, but nn.MaxPool2d stores the int values passed in (kernel_size=2, stride=2), so every vggnet forward raises TypeError: 'int' object is not subscriptable. _layer_padding has the same problem with the int `padding`. Once the int access works, the length update must also honor MaxPool2d's ceil_mode=True, which calculate_conv_output_size's floor division ignores. Fixes: - ConvSubsampling.forward: pass `lengths` to the masked stack when chunking is disabled; run 1-D stacks as a plain sequential (MaskedConvSequential.forward accepts lengths=None for exactly this) and report the precomputed out_lengths. - _forward_torch: read kernel/stride/padding via a _pair_first helper that tolerates int attributes, and add the ceil-mode remainder frame when a pooling layer runs with ceil_mode=True. Adds CPU unit tests for all three paths; each fails with TypeError or wrong lengths on the unfixed code. The chunking=-1 path is asserted to match the default (chunked) path bit-for-bit, and conv1d/vggnet lengths are asserted against calc_length references. Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
svcnvidia-nemo-ci
added
the
waiting-on-maintainers
|
/ok to test 738768e |
|
@codex review and check for potential for silent regressions when the caller doesn't provide lengths argument, but they should have |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 738768e608
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| x, lengths = self.conv(x, lengths) | ||
| else: | ||
| # 1-D conv stacks run without masking; keep the lengths computed above. | ||
| x = self.conv(x) |
There was a problem hiding this comment.
For striding_conv1d and dw_striding_conv1d with variable-length batches, this call deliberately omits lengths, so the new lengths=None branch processes padded frames as real input. With the symmetric stride-2 convolutions, padded values can affect even the final frame inside out_lengths (for example, the last valid output for length 97 and kernel size 5 reads positions beyond 96), silently making model results depend on batch padding. The new tests assert only shapes and calculated lengths, so they do not detect this; implement length-aware mask propagation for the 1-D layers and test invariance to changes beyond each sample's length.
AGENTS.md reference: AGENTS.md:L72-L78
Useful? React with 👍 / 👎.
| fuse_triton = False | ||
|
|
||
| def forward(self, x, lengths): | ||
| def forward(self, x, lengths=None): |
There was a problem hiding this comment.
Commit 1ed03a40f2fae9ab5a42146e4e902acfae763e47 has no Signed-off-by trailer, so it cannot be merged under the repository's DCO policy. Configure user.name and user.email to the real author identity, then repair the branch with git rebase --signoff origin/main and git push --force-with-lease.
AGENTS.md reference: AGENTS.md:L80-L90
Useful? React with 👍 / 👎.
|
good question, checked it before making the change:
so the silent-regression case would need someone to call the internal wrapper directly, skip lengths, and use the single-return contract, which is exactly the documented plain-pass-through mode. happy to make the wrapper raise on None for 2-D stacks instead if you'd rather have that belt and suspenders. |
svcnvidia-nemo-ci
added
waiting-on-maintainers
|
[🤖]: Hi @ManoharPaturi 👋, We wanted to let you know that a CICD pipeline for this PR just finished successfully. So it might be time to merge this PR or get some approvals. |
|
Hi @pzelasko, following up as the full CICD pipeline has completed successfully. Please let me know if any further changes are needed or if this is good to merge. Thanks! |
svcnvidia-nemo-ci
removed
the
waiting-on-maintainers
Fixes #16223.
Three repairs to paths the MaskedConvSequential refactor (#13827) missed:
MaskedConvSequential.forwardtakes optionallengths; 1-D stacks without length-aware layers just pass them throughConvSubsampling.forwardnow forwardslengthstoself.conv(...)in the non-chunked / conv1d branch (theelsethat previously calledself.conv(x))_layer_paddinghandlesnn.MaxPool2dstoring int kernel/stride/padding (via a_pair_firsthelper) and accounts for the extra frameceil_mode=Truecan addsubsampling_conv_chunking_factor=-1verified bit-identical to the default path for striding/dw_striding/vggnet at factors 4 and 8. 14 new CPU tests — all fail on main, pass here.